#13 Spreadsheet download #22
Conversation
…ingest-client into 13_spreadsheet-download
also DRY the data collector service
| self._flatten_object(value, flattened_object, parent_key=full_key) | ||
| else: | ||
| flattened_object[full_key] = str(value) | ||
| elif isinstance(object, list): |
There was a problem hiding this comment.
Should there be an else path as well that raises an exception to protect client from unsupported use?
There was a problem hiding this comment.
There's no need for that, if you check where this function is being called, it can only be called if it's a dict or list.
There was a problem hiding this comment.
Could it be called by someone else using different arguments?
| elif isinstance(object, list): | ||
| self._flatten_list(flattened_object, object, parent_key) | ||
|
|
||
| def _flatten_list(self, flattened_object, object, parent_key): |
There was a problem hiding this comment.
Why are some of these methods returning values and some modifying properties on the object without returning it? In general, I think it's bad practice for a function to modify an object since it can cause unexpected side effects. I think it would be better to copy the object, change it, and return it
There was a problem hiding this comment.
These are private functions so it shouldn't be a big concern. They are needed to be modified because it's part of the recursive logic. I'm not yet sure how to achieve it with your suggestion to copy the object, change it, and return it. I will think about it. Could we leave that for later?
There was a problem hiding this comment.
Yeah no problem. I was just curious more than anything. Being private makes sense
There was a problem hiding this comment.
I agree with @jacobwindsor that pure functions are an ideal to aspire to, since side0effects can be a source of surprise and therefore bugs. Sometimes, if we make an explicit decision to have side-effects, the function's name should make it clear.
ingest/downloader/downloader.py
Outdated
|
|
||
| def __add_row_content(self, worksheet, headers: list, values: dict): | ||
| @staticmethod | ||
| def __add_row_content(worksheet, headers: list, row_no: int, values: dict): |
There was a problem hiding this comment.
You can can call it row_number, or if you want to shorten, rownum.
closes ebi-ait/dcp-ingest-central#13
Changes: